Skip to content

Port number correction on client for SslStream sample (C#, VB) #4377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

DavidEshtehari
Copy link

Summary

The port number for negotiation with the server must be the same. I provided this PR to fix it and added a note to create the certificate with a private key.

@dotnet-bot dotnet-bot added this to the June 2020 milestone Jun 13, 2020
@DavidEshtehari DavidEshtehari changed the title Port number correction on client Port number correction on client for SslStream sample (C#, VB) Jun 13, 2020


> [!NOTE]
> Be sure you have the private key included in the certificate; cause the server needs it to decrypt the received message otherwise you will get a decryption error on the server-side.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> Be sure you have the private key included in the certificate; cause the server needs it to decrypt the received message otherwise you will get a decryption error on the server-side.
> Be sure to include the private key in the certificate. The server needs the private key to decrypt the received message. If you omit it, you'll get a decryption error on the server side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is true in general, Is this remark attached to some specific property?
Also I think the "message" should be plural. Perhaps something like ... decrypt the received messages and complete handshake.
However, AFAIK we will not get that far and SslStream will throw PNSP or Authentication Exception if X509Certificate without key is provided. That should be documented on the property it self and delegates returning certificates.
That being said, there is also some fixup code to search through certificate stores and it can work even if given certificate instance does not have a key. But it may be better to guide users to use X509Certificate2 properly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion. Any description to clarify the sample and its related issues is welcome.
Please, commit your suggestions. I'll accept it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to using the certificates is not the subject of this context. And I have avoided describing it more here. Nonetheless, if you have useful links we can add it here as a reference.

@gewarren gewarren requested a review from a team June 22, 2020 21:27
@@ -40,7 +40,7 @@ public static void RunClient(string machineName, string serverName)
//<snippet4>
// Create a TCP/IP client socket.
// machineName is the host running the server application.
TcpClient client = new TcpClient(machineName,443);
TcpClient client = new TcpClient(machineName,8080);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we change the port? 443 is default for ssl/tls while 80 and 8080 is typically used plain HTTP.
If this is a problem, perhaps the example should take the port as parameter.

Copy link
Author

@DavidEshtehari DavidEshtehari Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late back. If you look at the server-side will see there is a port over there also. The point is these two ports must be the same. I didn't choose the 443 to avoid the misunderstanding of using the standard https port number.
Using a parameter will make it complicated. So, I'd rather the plain numbers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we change the port? 443 is default for ssl/tls while 80 and 8080 is typically used plain HTTP.

If this is a problem, perhaps the example should take the port as parameter.

I do agree with the possible confusion using 80 plain HTTP related port.

As mentioned by @DavidEshtehari the server example is using 8080. Should we change both of them?

Using default port can also be good from my point of view.

We should only take care of that, perhaps those code snippets are used more than once and that kind of mismatch could be perhaps found somewhere else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of updating the server. Since the client uses the default port, it would just work and be representative of users actually need to do in most cases. And yes, it make sense that the examples work together e.g. use same port.

@gewarren
Copy link
Contributor

@DavidEshtehari It looks like there is some feedback to respond to before we can merge this.

DavidEshtehari and others added 2 commits July 26, 2020 20:21
@carlossanlop
Copy link
Contributor

@wfurt @gewarren can you please re-review this PR? The comments have been addressed.
Ping @dotnet/ncl too.

Base automatically changed from master to main March 5, 2021 20:52
@gewarren gewarren requested a review from a team as a code owner March 5, 2021 20:52
@krwq
Copy link
Member

krwq commented Jun 22, 2021

I might be misunderstanding something here but I think @wfurt's suggestion was to change server port to something which typically uses TLS by default (i.e. 443) and also make sure client matches same port

@jeffhandley jeffhandley modified the milestones: June 2020, Backlog Apr 29, 2022
@gewarren
Copy link
Contributor

gewarren commented Jun 5, 2024

@DavidEshtehari Do you still plan to work on this PR?

@BillWagner
Copy link
Member

closing as abandoned.

@BillWagner BillWagner closed this Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants